[improvement](be) Add CPU-aware S3 rate limiter#65420
Conversation
Add three new CPU-aware S3 limiter configs: - s3_get_token_per_second_per_core: GET QPS per CPU core (0 to disable) - s3_put_token_per_second_per_core: PUT QPS per CPU core (0 to disable) - token_per_second_max: Hard cap on derived QPS (0 means no cap) When per-core values are > 0, effective QPS = per_core_value * CpuInfo::num_cores() When per-core values are = 0, existing absolute configs are used (preserving backward compatibility). All new configs default to 0, maintaining existing behavior. Affected files: - be/src/common/config.h: Added DECLARE statements - be/src/common/config.cpp: Added DEFINE statements with validators Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fix naming inconsistency: all other S3 rate-limiter config options carry the s3_ prefix. This public API change must be fixed now before downstream code starts using these configs. - DECLARE_mInt64(s3_token_per_second_max) in config.h - DEFINE_mInt64(s3_token_per_second_max) in config.cpp - DEFINE_Validator(s3_token_per_second_max) in config.cpp Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add S3RateLimiterConfig struct and get_effective_s3_rate_limiter_config() to centralize the logic for computing effective rate limiter parameters from both legacy configs and new per-core configs. Changes: - Add S3RateLimiterConfig struct in s3_util.h - Add get_effective_s3_rate_limiter_config() declaration and implementation - Add helper functions checked_positive_product() and apply_s3_token_per_second_max() - Add required includes: <limits> and util/cpu_info.h This is Task 2 of the S3 rate limiter refactoring. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n and dynamic reset
Replace raw config::s3_{get,put}_* accesses with get_effective_s3_rate_limiter_config()
in both S3ClientFactory constructor (initialization) and check_s3_rate_limiter_config_changed()
(dynamic reset). This ensures per-core QPS scaling and the s3_token_per_second_max cap are
applied consistently at startup and on every dynamic config check.
Also add an INFO log on successful limiter reset to make config changes observable in logs.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cover four scenarios: absolute GET config, per-core GET without cap, per-core GET with cap, and per-core PUT with cap. Add S3RateLimiterConfigGuard RAII helper to save/restore all nine related configs after each test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replace manual save/restore of 3 config values with S3RateLimiterConfigGuard that handles all 9 relevant S3 rate limiter configs. This prevents state contamination between tests. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…r-core and cap config
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: CPU-aware S3 rate limiter calculation multiplied per-core QPS by BE CPU cores before applying s3_token_per_second_max. Extremely large mutable per-core settings could overflow int64 and trigger a CHECK even when a valid max cap was configured. This change computes the CPU-derived QPS with a wider intermediate value and applies the configured cap safely. It also adds unit coverage for the overflow-with-cap case and PUT limiter rebuild when the max cap changes.
### Release note
None
### Check List (For Author)
- Test: Unit Test
- ./run-be-ut.sh --run --filter=S3UTILTest.effective_s3_rate_limiter_config_caps_overflowing_per_core_get_config
- ./run-be-ut.sh --run --filter='S3UTILTest.*'
- Behavior changed: No
- Does this need documentation: No
5287ce9 to
a4effbe
Compare
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: The S3 bytes limiter ignored the result of add(), unlike the request QPS limiter, and the per-core bandwidth configs defaulted to disabled. Check the bytes limiter result consistently and set GET/PUT per-core defaults to 100 MiB/s. ### Release note Set default per-core S3 GET/PUT bandwidth limits to 100 MiB/s and consistently handle limiter failures. ### Check List (For Author) - Test: No need to test (per user request; format checks only) - Behavior changed: Yes (S3 bandwidth limiting defaults to 100 MiB/s per CPU core) - Does this need documentation: No
|
/review |
There was a problem hiding this comment.
Found one blocking issue in the S3 bandwidth limiter propagation.
Critical checkpoint conclusions:
- Goal/test coverage: The PR adds CPU-aware S3 request and byte limiters and BE unit tests for config calculation and dynamic rebuild. It does not fully apply the new byte limiter to every object-storage path that already shares the S3 rate-limiter contract.
- Scope/focus: The core PR scope is focused on S3 limiter config, S3 client enforcement, and tests.
- Concurrency/lifecycle: Limiter reset keeps the existing holder locking and atomic update guards; I did not find a separate concurrency issue in the authoritative PR file set.
- Config/compatibility: The new mutable BE configs are dynamically observed through the existing checker, but the shared Azure path is left incompatible with the new bandwidth limiter behavior.
- Parallel paths: Native S3 charges bytes for PUT/UploadPart/GetObject; Azure still charges only request count through the same shared limiter gate.
- Tests/CI: The added unit tests cover S3 effective config and rebuild behavior, but not the Azure parallel path. I could not run BE UTs in this runner because the checkout lacks thirdparty/installed and protoc. git diff --check was clean for the GitHub PR files; check-format.sh could not run because clang-format 16 is unavailable.
User focus: no additional user-provided review focus was supplied.
| if (sleep_duration < 0) { | ||
| return T(s3_error_factory()); | ||
| } | ||
| if (bytes > 0) { |
There was a problem hiding this comment.
The byte limiter only gets charged on the native S3 path. Azure still uses the same enable_s3_rate_limiter/S3ClientFactory::rate_limiter() helper, but its put_object, upload_part, and get_object calls never pass stream.size() or bytes_read through bytes_rate_limiter(). With the new positive defaults for s3_get_bytes_per_second_per_core and s3_put_bytes_per_second_per_core, enabling the rate limiter now throttles S3 bandwidth but silently leaves Azure bandwidth unlimited. Please either apply the same byte accounting to the Azure read/write helpers or make these new configs/API explicitly S3-native so Azure users do not expect them to apply.
### What problem does this PR solve? Issue Number: N/A Related PR: apache#65420 Problem Summary: The CPU-aware S3 bandwidth limiter was applied only to the native S3 client. Azure shared the request limiter but did not charge bytes for uploads or downloads, so enabling the limiter produced inconsistent behavior. Centralize request and byte limiter acquisition and pass payload sizes from Azure put, multipart upload, and get operations. ### Release note Apply object-storage bandwidth limits consistently to native S3 and Azure Blob Storage. ### Check List (For Author) - Test: Unit Test - `S3UTILTest.*` - Behavior changed: Yes. Azure Blob Storage data transfers now consume the shared byte-rate limiter when S3 rate limiting is enabled. - Does this need documentation: No
|
run buildall |
|
/review |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Automated review completed for PR #65420.
I did not find a new blocking issue to submit as an inline comment. The earlier Azure byte-accounting gap is already covered by the existing inline thread discussion_r3557395816, and the current head now routes Azure put_object, upload_part, and get_object payload sizes through the shared acquire_s3_rate_limit path.
Critical checkpoint conclusions:
- Goal/test proof: the PR adds CPU-aware S3 request limits plus byte-rate limiting and centralizes effective limiter calculation. The added BE unit tests cover absolute/per-core/cap/dynamic-reset cases; I could not complete a local BE UT run in this checkout.
- Scope: the BE changes are focused around S3/Azure object-storage limiter config, helper propagation, and tests.
- Concurrency/lifecycle: limiter reset continues to use
TokenBucketRateLimiterHolderlocks; startup use ofCpuInfo::num_cores()is after BECpuInfo::init(), and the BE test harness also initializesCpuInfo. - Config/dynamic behavior: new mutable configs have non-negative validators, and
check_s3_rate_limiter_config_changed()observes request and byte limiter changes. - Compatibility/parallel paths: native S3 and Azure transfer paths now both use the shared request+byte limiter helper. Cloud recycler has its own config surface and was not changed by this BE-scoped PR.
- Tests/style:
git diff --checkis clean. I attempted./run-be-ut.sh --run --filter='S3UTILTest.*', but interrupted it while it was still bootstrapping dependencies/submodules before reaching test execution. - User focus: no additional user-provided focus points.
Subagent conclusions:
optimizer-rewrite: no optimizer/Nereids surface and no indirect semantic issue found.tests-session-config: no new tests/config/CI candidate found; existing Azure byte-accounting concern is duplicate-covered and fixed in head.- Convergence round 1 ended with both live subagents replying
NO_NEW_VALUABLE_FINDINGSfor the same final ledger/comment set.
TPC-H: Total hot run time: 29332 ms |
TPC-DS: Total hot run time: 180020 ms |
ClickBench: Total hot run time: 24.93 s |
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
The existing S3 GET/PUT token bucket rate limiter uses fixed per-BE QPS settings. This is not suitable for heterogeneous deployments because a BE with many CPU cores and a BE with few CPU cores receive the same S3 request budget.
This PR adds CPU-aware S3 rate limiter settings for GET and PUT requests. When the new per-core settings are left as 0, Doris keeps the existing absolute limiter behavior. When a per-core setting is positive, Doris derives the effective per-BE S3 QPS from BE CPU cores and caps it with s3_token_per_second_max when configured.
The PR also centralizes effective limiter config calculation, reuses it for initialization and dynamic rebuild, and guards CPU-derived QPS calculation from overflow before applying the max cap.
Release note
Add BE configs to support CPU-aware S3 GET/PUT request rate limiting.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)